Support icechunk #101
Support icechunk #101
Conversation
Co-authored-by: Henry Rodman <[email protected]>
|
Just added auth for the RASI data and it immedately worked with titiler-multidim! RASI map |
| - name: Run tests | ||
| run: uv run pytest | ||
| run: uv run pytest -n auto |
There was a problem hiding this comment.
Added pytest-xdist mostly for more performant tests locally. Does not do much on a 2 core gh runner.
Remove installation of system dependencies for compilation.
There was a problem hiding this comment.
I should probably take those out?
src/titiler/multidim/reader.py
Outdated
|
|
||
| authorize_virtual_chunk_access = authorize_virtual_chunk_access or {} | ||
|
|
||
| print(f"DEBUG: authorize_virtual_chunk_access = {authorize_virtual_chunk_access}") |
There was a problem hiding this comment.
| print(f"DEBUG: authorize_virtual_chunk_access = {authorize_virtual_chunk_access}") |
| assert response.status_code == 200 | ||
| assert response.json() == ds_params["variables"] | ||
| # TODO: Do we care about the order? |
There was a problem hiding this comment.
@hrodmn just a flag here. I think we don't but let me know if I am missing anything here.
There was a problem hiding this comment.
I don't think the order matters!
|
|
||
| [dependency-groups] | ||
| dev = [ | ||
| "dask>=2025.9.1", |
There was a problem hiding this comment.
I think this was beneficial for generating the test fixtures, but happy to remove it if you think that I should.
There was a problem hiding this comment.
I think it is fine to include it in the dev dependencies
|
@hrodmn this is ready for a review! Please ignore the tons of files in the test fixtures (sorry they clobber the diff quite a bit). |
hrodmn
left a comment
hrodmn
left a comment
There was a problem hiding this comment.
I think this is great - nice work @jbusecke! The only changes I request are to avoid using assertions for raising errors in the actual runtime code.
For context on the configuration and deployment, the value that we set for TITILER_MULTIDIM_AUTHORIZED_CHUNK_ACCESS will be added as a secret value in a SSM secret that gets shared by multiple applications in a given deploy environment, so we may only have to define it once per deployment environment even if multiple applications want to use the same values.
titiler-multidim/.github/actions/cdk-deploy/action.yml
Lines 50 to 61 in 31c20aa
|
|
||
| [dependency-groups] | ||
| dev = [ | ||
| "dask>=2025.9.1", |
There was a problem hiding this comment.
I think it is fine to include it in the dev dependencies
| authorize_virtual_chunk_access: Optional[Dict[str, Dict[str, Any]]] = None, | ||
| ) -> xr.Dataset: | ||
| """Open an IceChunk dataset using xarray.""" | ||
| assert icechunk is not None, "'icechunk' must be installed to read icechunk dataset" |
There was a problem hiding this comment.
Outside of tests I would raise actual exceptions (like ImportError in this case) instead of assert statements. If the application gets run in "optimized mode" the assert statements will be ignored!
| ) -> List[str]: | ||
| """List available variable in a dataset.""" | ||
| with xarray_open_dataset( | ||
| # todo: why is this not a method of the reader class? Seems like a smell that I have to define the opener here again.... |
There was a problem hiding this comment.
Maybe we could add a XarrayReader.open_src method and use that in XarrayReader.__post_init__ and here so we could at least recycle the code.
| assert response.status_code == 200 | ||
| assert response.json() == ds_params["variables"] | ||
| # TODO: Do we care about the order? |
There was a problem hiding this comment.
I don't think the order matters!
|
Thanks so much for the review @hrodmn. Ill see if I can get this cleaned up and ready for Monday! |
Replacing #96 since we cannot deploy to sandbox from a fork.
Depends on #95 and developmentseed/titiler#1235
Tasks